Skip to content

accounts/external: handle 0 chainid as not-set for the Clef API#23394

Merged
karalabe merged 2 commits into
ethereum:masterfrom
karalabe:fix-clef-0-chainid
Aug 13, 2021
Merged

accounts/external: handle 0 chainid as not-set for the Clef API#23394
karalabe merged 2 commits into
ethereum:masterfrom
karalabe:fix-clef-0-chainid

Conversation

@karalabe
Copy link
Copy Markdown
Member

No description provided.

@karalabe karalabe added this to the 1.10.8 milestone Aug 13, 2021
@karalabe karalabe requested review from fjl and holiman August 13, 2021 09:55
Comment thread accounts/external/backend.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw, the canonical zero check we use everywhere is int.Sign() == 0.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PTAL

Comment thread accounts/external/backend.go Outdated
Copy link
Copy Markdown
Contributor

@fjl fjl Aug 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, tx.ChainId can never be nil.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PTAL

// However, if the user asked for a particular chain id, then we should
// use that instead.
if tx.ChainId() != nil {
if tx.ChainId().Sign() != 0 {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we need a nil-check here?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ChainId() cannot return nil for non-legacy transactions.

Copy link
Copy Markdown
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@karalabe karalabe merged commit 0a68558 into ethereum:master Aug 13, 2021
atif-konasl pushed a commit to frozeman/pandora-execution-engine that referenced this pull request Oct 15, 2021
…reum#23394)

* accounts/external: handle 0 chainid as not-set for the Clef API

* accounts/external: document SignTx

Co-authored-by: Felix Lange <fjl@twurst.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants